Feature - Identity verification implementation #2196

jinliu9508 · 2024-09-26T17:44:43Z

Description

One Line Summary

Implement identity verification functionality to our Android SDK with JWT (JSON Web Token) that manage a specific User, their Subscriptions, and Identities, if enabled using OneSignal dashboard.

Details

Motivation

OneSignal Identity Verification feature only exists today for the Player model and we want to bring this feature to the User Model and gives us an opportunity to switch to a more standard client security model, JWT (JSON Web Token).

For Developers

If identity verification is enabled, all operations requiring identity or authorization must be accompanied by a correct JWT. Developers can use OneSignal.login(external_id, jwt) to set a JWT for a specific user identified by their external ID, or OneSignal.updateUserJwt(external_id, jwt) to update the JWT for that user. Additionally, developers can add a JWTInvalidatedListener by calling OneSignal.addUserJwtInvalidatedListener, allowing them to listen for a JWTInvalidatedEvent triggered when a JWT is invalidated by any operation, and subsequently update the JWT as necessary.

To Help With Review

This PR contains a some major structural changes working around identity verification.

New public API methods addUserJwtInvalidatedListener and updateUserJwt in UserManager.
Certain backend services and operations are now able to make request with JWT or deviceAuthPushToken depending on the operation type and purpose. They can also handle the UNAUTHORIZED http response and fire onUserJwtInvalidated callback accordingly.
Optional headers like JWT and deviceAuthPushToken can now be passed to HttpClient to be included in each request.
OperationRepo behavior change depends on the status of identity verification
initWithContext behavior change: switching user is now placed after the remote params fetch. Creating anonymous user or subscription when identity verification is on will suppress backend operations.
Login behavior change: update JWT for all future requests from the user.
Logout behavior change: suspend the push subscription

Testing

Unit testing

I have include a testing case that simulate a UNAUTHROIZED error (401, 403) and ensure that the jwt for the specific user is invalidated and the JWTInvalidatedEvent is fired.

Manual testing

Fresh Install: An anonymous user is created upon a fresh install and will not create a login request until Login with a JWT is called.
Upgrade from User Model:
- If a user is logged prior the upgrade and does not have JWT, app will fire the callback to retrieve a JWT and then attempt to send the request.
- If the user has JWT cached, app will directly send a login request.
- If no user has previously logged in successfully, the app will work the same as fresh install.
When an UNAUTHORIZED http error is caught: onJwtInvalidatedEvent will be fired and the code that retrieves a new JWT in UserJwtInvalidatedListener will be called to update user JWT. The request will then retry with the new JWT.

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
- If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
Any Public API changes are explained in the PR details and conform to existing APIs

Testing

I have included test coverage for these changes, or explained why they are not needed
All automated tests pass, or I explained why that is not possible
I have personally tested this on my device, or explained why that is not possible

Final pass

Code is as readable as possible.
- Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
I have reviewed this PR myself, ensuring it meets each checklist item
- WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is

jkasten2

Most of my feedback was added inline, but I have one additional comment that wasn't clear if it is TODO yet or not. Is the new JWT endpoint to remove email and SMS by token still TODO in this PR, or did I miss it?

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/application/MainApplication.java

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/model/MainActivityViewModel.java

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/util/SharedPreferenceUtil.java

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/IUserJwtInvalidatedListener.kt

jkasten2 · 2024-10-25T02:34:59Z

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/identity/IdentityModel.kt

+    var jwtToken: String?
+        get() = getOptStringProperty(IdentityConstants.JWT_TOKEN)
+        set(value) {
+            setOptStringProperty(IdentityConstants.JWT_TOKEN, value, ModelChangeTags.NO_PROPOGATE)


Why don't we want to propagate?

Propagating the property will result in a setAlias request. We don't want the change of JWT triggers a setAlias operation.

...lSDK/onesignal/core/src/main/java/com/onesignal/user/internal/identity/IdentityModelStore.kt

...signal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt

jkasten2 · 2024-10-25T03:00:18Z

...signal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt

@@ -82,8 +84,10 @@ internal class LoginUserFromSubscriptionOperationExecutor(
            return when (responseType) {
                NetworkUtils.ResponseStatusType.RETRYABLE ->
                    ExecutionResponse(ExecutionResult.FAIL_RETRY)
-                NetworkUtils.ResponseStatusType.UNAUTHORIZED ->
+                NetworkUtils.ResponseStatusType.UNAUTHORIZED -> {
+                    _identityModelStore.invalidateJwt()


I see we call this _identityModelStore.invalidateJwt() in every executor, do you think it would work out to have the OperationRepo process ExecutionResult.FAIL_UNAUTHORIZED so it is the only one calling invalidateJwt()?

Addressed in the fixup commit. The original plan was to fire the invalidate event for some specific operations instead of any operation. but I just realized UNAUTHORIZED response can only be returned by these specific operations anyway. I just move the invalidation to OperationRepo out of executors.

jkasten2 · 2024-10-25T03:07:45Z

...ain/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt

@@ -160,7 +164,18 @@ internal class LoginUserOperationExecutor(

        try {
            val subscriptionList = subscriptions.toList()
-            val response = _userBackend.createUser(createUserOperation.appId, identities, subscriptionList.map { it.second }, properties)
+            val pushSubscription = subscriptions.values.find { it.type == SubscriptionObjectType.ANDROID_PUSH }


This SDK supports fireOS and Huawei push so you will need a more complete check. However, since this looks like it is for Device Auth then we can remove it.

That is, once we have 100% committed to that path, so hold on addressing this comment until then.

The only purpose for the pushSubscription is to acquire the push token. Since we are confirmed that we don't need the token for subscription operation, I removed it in the fixup commit.

I see you removed the usage of this but this line was not cleaned up.

fixup

…tions that have no valid JWT

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/model/MainActivityViewModel.java

jkasten2 · 2024-11-18T23:39:23Z

...ignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt

@@ -72,7 +72,7 @@ internal class ConfigModelStoreListener(
                    // copy current model into new model, then override with what comes down.
                    val config = ConfigModel()
                    config.initializeFromModel(null, _configModelStore.model)
-
+                    config.fetchParamsNotifier = _configModelStore.model.fetchParamsNotifier


I see, you are right, I mixed up what was happening here.

After looking at this again this extra event seems unnecessary, as we already have a onModelReplaced event that can be listen to instead.

...nalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt

...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

...sages/src/test/java/com/onesignal/inAppMessages/internal/backend/InAppBackendServiceTests.kt

jkasten2 · 2024-11-20T22:48:18Z

...ain/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt

@@ -160,7 +164,18 @@ internal class LoginUserOperationExecutor(

        try {
            val subscriptionList = subscriptions.toList()
-            val response = _userBackend.createUser(createUserOperation.appId, identities, subscriptionList.map { it.second }, properties)
+            val pushSubscription = subscriptions.values.find { it.type == SubscriptionObjectType.ANDROID_PUSH }


I see you removed the usage of this but this line was not cleaned up.

jkasten2 · 2024-11-20T22:53:16Z

...ignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt

@@ -72,7 +72,7 @@ internal class ConfigModelStoreListener(
                    // copy current model into new model, then override with what comes down.
                    val config = ConfigModel()
                    config.initializeFromModel(null, _configModelStore.model)
-
+                    config.fetchParamsNotifier = _configModelStore.model.fetchParamsNotifier


I think the IdentityVerificationService you created is a correct pattern, however I believe it could have just used onModelReplaced, instead of needing to introduce fetchParamsNotifier. Switching to that means less code and follows other patterns.

However if your fetchParamsNotifier handles something I am not seeing let me know, or you feel it is a better pattern here.

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt

jinliu9508 requested review from jkasten2, nan-li and emawby September 26, 2024 17:45

jinliu9508 changed the title ~~Identity verification implementation~~ [WIP] Identity verification implementation Sep 26, 2024

jinliu9508 changed the title ~~[WIP] Identity verification implementation~~ [WIP] Feature - Identity verification implementation Sep 26, 2024

jinliu9508 force-pushed the identity-verification-implementation branch 9 times, most recently from a6e4349 to ff50c7d Compare October 9, 2024 20:13

jinliu9508 force-pushed the identity-verification-implementation branch 3 times, most recently from 95c6162 to 8931712 Compare October 15, 2024 17:27

jkasten2 requested changes Oct 25, 2024

View reviewed changes

jinliu9508 force-pushed the identity-verification-implementation branch from 8931712 to ffa634a Compare November 5, 2024 19:15

jinliu9508 added 11 commits November 7, 2024 13:25

Infrastructure change around JWT: models, model stores

3caffc9

Public API impl for JWT, User Manager, and callbacks

c588498

Add JWT to HttpClient, backend services, and operations

bf759d2

fixup

Add UNAUTHORIZED handling to OperationRepo and operation executors

f29d0df

Add and adjust some test units for JWT related change

f8abc51

Add UI element to DemoApp to manually test JWT impl

9e13707

Manually test with a specific app in staging

eb525f4

OperationRepo: when identity verification is on, do not process opera…

76b95d0

…tions that have no valid JWT

Disable operations that are not allowed when identity verification is on

034e9c9

use new name from remote params

af3ba8a

Init behavior change based on identity verification

3b26b66

jinliu9508 added 2 commits November 8, 2024 13:07

Add deviceAuthToken to subscription requests

3083eae

calling logout disable push subscription

db78c39

jinliu9508 force-pushed the identity-verification-implementation branch from ffa634a to db78c39 Compare November 8, 2024 18:07

jinliu9508 force-pushed the identity-verification-implementation branch 2 times, most recently from 8e9ae06 to 868d5d4 Compare November 18, 2024 21:19

jinliu9508 changed the title ~~[WIP] Feature - Identity verification implementation~~ Feature - Identity verification implementation Nov 18, 2024

jkasten2 requested changes Nov 18, 2024

View reviewed changes

jinliu9508 force-pushed the identity-verification-implementation branch 2 times, most recently from 1b9491f to 9d70a5c Compare November 20, 2024 20:51

jkasten2 requested changes Nov 20, 2024

View reviewed changes

jinliu9508 force-pushed the identity-verification-implementation branch from 9d70a5c to e39e8bd Compare November 21, 2024 19:04

fixup: identity verification improvement

48f4c9a

jinliu9508 force-pushed the identity-verification-implementation branch from e39e8bd to 48f4c9a Compare November 21, 2024 21:06

jkasten2 approved these changes Nov 21, 2024

View reviewed changes

jinliu9508 changed the base branch from main to identity_verification_beta November 22, 2024 16:41

jinliu9508 merged commit b029886 into identity_verification_beta Nov 22, 2024
2 checks passed

jinliu9508 deleted the identity-verification-implementation branch November 22, 2024 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature - Identity verification implementation #2196

Feature - Identity verification implementation #2196

jinliu9508 commented Sep 26, 2024 •

edited by niels-s

Loading

jkasten2 left a comment •

edited

Loading

jkasten2 Oct 25, 2024

jinliu9508 Nov 18, 2024

jkasten2 Oct 25, 2024

jinliu9508 Nov 18, 2024

jkasten2 Oct 25, 2024

jinliu9508 Nov 18, 2024

jkasten2 Nov 20, 2024

jkasten2 Nov 18, 2024

jkasten2 Nov 20, 2024

jkasten2 Nov 20, 2024 •

edited

Loading

Feature - Identity verification implementation #2196

Feature - Identity verification implementation #2196

Conversation

jinliu9508 commented Sep 26, 2024 • edited by niels-s Loading

Description

One Line Summary

Details

Motivation

For Developers

To Help With Review

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

jkasten2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkasten2 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

jinliu9508 commented Sep 26, 2024 •

edited by niels-s

Loading

jkasten2 left a comment •

edited

Loading

jkasten2 Nov 20, 2024 •

edited

Loading